Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @specifiedBy directive #2276

Merged
merged 13 commits into from
May 7, 2020
Merged

Conversation

m14t
Copy link
Contributor

@m14t m14t commented Dec 6, 2019

Description

This in an implementation for a spec proposal:

Example usage

Source

const { graphql, buildSchema } = require("graphql");

const schema = buildSchema(`
  """
  A UUID
  """
  scalar UUID @specified(by: "https://tools.ietf.org/html/rfc4122")
  
  type Query {
    someUUID: UUID
  }
`);

const root = {
  someUUID: () => {
    return "750fce86-fc2f-4b25-a7df-fd0941d3fa08";
  }
};

const query = `
{
  __type(name: "UUID") {
    kind
    name
    description
    specifiedBy
  }
}`;

graphql(schema, query, root).then(response => {
  console.log(JSON.stringify(response, null, 4));
});

Output

{
    "data": {
        "__type": {
            "kind": "SCALAR",
            "name": "UUID",
            "description": "A UUID",
            "specifiedBy": "https://tools.ietf.org/html/rfc4122"
        }
    }
}

Notes

Please let me know if I forgot something.

@IvanGoncharov IvanGoncharov added spec RFC Implementation of a proposed change to the GraphQL specification PR: feature 🚀 requires increase of "minor" version number labels Dec 7, 2019
@m14t
Copy link
Contributor Author

m14t commented Dec 7, 2019

@IvanGoncharov Thanks for the review!

While trying to improve the test coverage here, I wanted pass an SDL that used @specified to cycleSDL, but ran into #2020.

I'm not entirely sure of the implications of this, but if more work is need and given some direction, I'd be happy to continue to work on this.

cc: @eapache

Copy link
Member

@eapache eapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak to the question about schema printer but the rest of this LGTM.

Thanks for the help Matt!

src/type/directives.js Outdated Show resolved Hide resolved
@IvanGoncharov
Copy link
Member

While trying to improve the test coverage here, I wanted pass an SDL that used @Specified to cycleSDL, but ran into #2020.

@m14t It should be handled the same way as @deprecate.
#2020 is about printing user-provided directives and there is nothing wrong in hardcoding standard directives for a time being.
Also please review tests that mention @deprecate or deprecatedReason and replicate ones that make sense for @specified/specifiedBy.

@m14t m14t force-pushed the specified-directive branch 2 times, most recently from 8bb758d to e76581b Compare December 16, 2019 03:09
@m14t
Copy link
Contributor Author

m14t commented Dec 16, 2019

@IvanGoncharov thank you for the feedback.

I've made the requested changes, and feel better about this. I still need to check that I have all of the test cases, but think that the implementation is done.

@IvanGoncharov
Copy link
Member

I still need to check that I have all of the test cases, but think that the implementation is done.

@m14t You should have 100% coverage for diff ATM it's 97.56%:
image

@m14t m14t force-pushed the specified-directive branch 4 times, most recently from 74bb0d3 to 90e650e Compare December 31, 2019 02:18
m14t added a commit to m14t/graphql-wg that referenced this pull request Jan 2, 2020
I've been working on the [implementation](graphql/graphql-js#2276) of the `@specified(by: "")` directive as specified by graphql/graphql-spec#649, and am interested an any feedback or progress on that effort.
@IvanGoncharov
Copy link
Member

@m14t Can please update this PR to be in sync with graphql/graphql-spec@b418b60 ?

m14t added a commit to m14t/graphql-wg that referenced this pull request Jan 9, 2020
I've been working on the [implementation](graphql/graphql-js#2276) of the `@specified(by: "")` directive as specified by graphql/graphql-spec#649, and am interested an any feedback or progress on that effort.
@m14t m14t changed the title Add @specified directive Add @specifiedBy directive Feb 5, 2020
@m14t
Copy link
Contributor Author

m14t commented Apr 25, 2020

@IvanGoncharov Sorry for the delay here, but this is ready for another review when you have a chance.

@m14t m14t force-pushed the specified-directive branch 2 times, most recently from 4dcf49b to c4a6a0b Compare April 28, 2020 15:29
@IvanGoncharov
Copy link
Member

@m14t Sorry for the delay will try to review, merge, release before tomorrow's WG.

@@ -166,6 +175,7 @@ export type IntrospectionScalarType = {|
+kind: 'SCALAR',
+name: string,
+description?: ?string,
+specifiedByUrl: ?string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a breaking change: code that was using IntrospectionScalarType no longer type-checks unless specifiedByUrl is explicitly set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants